Conversation
- `DbConnectionBuilder::build` becomes async without tokio's block_in_place. Still need to add `web` feature flag.
Renamed the `run_threaded` method on `wasm32` to better reflect its behavior of spawning a background task. The generated `DbConnection` methods `run_threaded`, `run_background`, and `advance_one_message_blocking` now include runtime panics with a clear error feedback when called on unsupported targets.
Trim down repetitive `cfg` clauses by extracting common lock patterns into `get_lock_[sync|async]`.
Moves the creation of DbContextImplInner and DbContextImpl into private helper functions (`build_db_ctx_inner` and `build_db_ctx`) to reduce duplication between the web and non-web implementations of `build_impl`.
Signed-off-by: Zeke Foppa <196249+bfops@users.noreply.github.com>
Signed-off-by: Zeke Foppa <196249+bfops@users.noreply.github.com>
…worklabs/SpacetimeDB into bfops/wasm-test
Signed-off-by: Zeke Foppa <196249+bfops@users.noreply.github.com>
…worklabs/SpacetimeDB into bfops/wasm-test
| ) | ||
| .await; | ||
|
|
||
| ctr_for_subs.wait_for_all().await; |
There was a problem hiding this comment.
these had to get moved out of the internal synchronous callback because in wasm everything runs in a single thread. Waiting for this inside the callback caused everything to lock up.
| ) | ||
| .await; | ||
|
|
||
| ctr_for_subs.wait_for_all().await; |
There was a problem hiding this comment.
these had to get moved out of the internal synchronous callback because in wasm everything runs in a single thread. Waiting for this inside the callback caused everything to lock up.
| check_val(row.n, 24) | ||
| }; | ||
| (one_u16_inserted.take().unwrap())(run_checks()); | ||
| let pre_ins_counter = pre_ins_counter.clone(); |
There was a problem hiding this comment.
small shimmy to deal with the different async semantics in wasm. we have to kick off both things and then await both.
…o bfops/wasm-update
…dk-updated' into bfops/wasm-test
|
Hey zeke, cool to see this moving forward :> |
| // These types determine the size of [`parse_expr`]'s stack frame. | ||
| // These types determine the size of [`parse_expr`]'s stack frame on 64-bit targets. | ||
| // Changing their sizes will require updating the recursion limit to avoid stack overflows. | ||
| // wasm32 has different type layouts, so this guard does not apply there. |
There was a problem hiding this comment.
It is true that 32-bit targets, including wasm32, will have different layouts, and so these assertions will not be accurate. It's odd that they matter at all, 'cause I wouldn't expect this crate to be a part of the dependency tree for spacetimedb-sdk, so I wouldn't expect it to ever get built for wasm32. If we were compiling SpacetimeDB (not just the SDK) for 32-bit targets and we cared at all about its performance in those contexts, we'd probably also want to add an assertion about the size of these things on 32-bit targets. But, you know, we aren't and we don't, so let's not.
| @@ -103,11 +107,20 @@ pub struct Test { | |||
| /// - `SPACETIME_SDK_TEST_CLIENT_PROJECT` bound to the `client_project` path. | |||
| /// - `SPACETIME_SDK_TEST_DB_NAME` bound to the database identity or name. | |||
| run_command: String, | |||
|
|
|||
| client_runner: ClientRunner, | |||
There was a problem hiding this comment.
What is this? What's it for? It looks like it's selecting between native clients and node clients, but if that's the case, why are we not just making the run_command be node my_wasm_file or whatever the hell?
There was a problem hiding this comment.
sounds great to me, thank you!
| ClientRunner::Web { | ||
| wasm_path, | ||
| bindgen_out_dir, | ||
| } => { | ||
| let rust_log = | ||
| "spacetimedb=debug,spacetimedb_client_api=debug,spacetimedb_lib=debug,spacetimedb_standalone=debug"; | ||
|
|
||
| let wasm_path = Path::new(wasm_path); | ||
| let bindgen_out_dir = PathBuf::from(bindgen_out_dir); | ||
| let bindgen_out_dir = if bindgen_out_dir.is_absolute() { | ||
| bindgen_out_dir | ||
| } else { | ||
| Path::new(client_project).join(bindgen_out_dir) | ||
| }; | ||
|
|
||
| create_dir_all(&bindgen_out_dir).expect("Failed to create wasm-bindgen out dir"); | ||
|
|
||
| // TODO: Make a browser-faithful wasm runner. | ||
| // `--target nodejs` is good enough for websocket and callback coverage, but it | ||
| // does not exercise browser-only behavior such as cookies, `LocalStorage`, or | ||
| // `SessionStorage`. Tests whose point is persisted web auth state should run in | ||
| // a real browser context rather than through this Node shim. | ||
| let output = cmd( | ||
| "wasm-bindgen", | ||
| [ | ||
| "--target".to_owned(), | ||
| "nodejs".to_owned(), | ||
| "--out-dir".to_owned(), | ||
| bindgen_out_dir | ||
| .to_str() | ||
| .expect("bindgen_out_dir should be valid utf-8") | ||
| .to_owned(), | ||
| wasm_path.to_str().expect("wasm_path should be valid utf-8").to_owned(), | ||
| ], | ||
| ) | ||
| .dir(client_project) | ||
| .stderr_to_stdout() | ||
| .stdout_capture() | ||
| .unchecked() | ||
| .run() | ||
| .expect("Error running wasm-bindgen"); | ||
| status_ok_or_panic(output, "wasm-bindgen", "(wasm-bindgen)"); | ||
|
|
||
| let js_module_name = wasm_path | ||
| .file_stem() | ||
| .expect("wasm_path should have a filename stem") | ||
| .to_str() | ||
| .expect("wasm_path stem should be valid utf-8"); | ||
| let js_module = bindgen_out_dir.join(format!("{js_module_name}.js")); | ||
| let js_module_cjs = bindgen_out_dir.join(format!("{js_module_name}.cjs")); | ||
| copy(&js_module, &js_module_cjs).expect("Failed to create .cjs wrapper for wasm-bindgen output"); | ||
| let js_module = js_module_cjs | ||
| .to_str() | ||
| .expect("js_module path should be valid utf-8") | ||
| .to_owned(); | ||
|
|
||
| let node_script = format!( | ||
| concat!( | ||
| "(async () => {{\n", | ||
| " const m = require({js_module:?});\n", | ||
| " if (m.default) {{ await m.default(); }}\n", | ||
| " const run = m.run || m.main || m.start;\n", | ||
| " if (!run) throw new Error('No exported run/main/start function from wasm module');\n", | ||
| " const runSelector = process.env.{TEST_RUN_SELECTOR_ENV_VAR} ?? '';\n", | ||
| " const dbName = process.env.{TEST_DB_NAME_ENV_VAR};\n", | ||
| " if (!dbName) throw new Error('Missing {TEST_DB_NAME_ENV_VAR}');\n", | ||
| " await run(runSelector, dbName);\n", | ||
| // These wasm clients run under Node rather than a browser. Some tests intentionally leave | ||
| // websocket/event-loop work alive once their assertions are complete, so exit here to keep | ||
| // non-lifecycle tests from hanging on leftover handles after `run()` has finished. | ||
| " process.exit(0);\n", | ||
| "}})().catch((e) => {{ console.error(e); process.exit(1); }});" | ||
| ), | ||
| js_module = js_module, | ||
| TEST_RUN_SELECTOR_ENV_VAR = TEST_RUN_SELECTOR_ENV_VAR, | ||
| TEST_DB_NAME_ENV_VAR = TEST_DB_NAME_ENV_VAR, | ||
| ); | ||
|
|
||
| let node_args: Vec<String> = vec!["--experimental-websocket".to_owned(), "-e".to_owned(), node_script]; | ||
|
|
||
| let output = cmd("node", node_args) | ||
| .dir(&bindgen_out_dir) | ||
| .env(TEST_CLIENT_PROJECT_ENV_VAR, client_project) | ||
| .env(TEST_DB_NAME_ENV_VAR, db_name) | ||
| .env(TEST_RUN_SELECTOR_ENV_VAR, run_command) | ||
| .env("RUST_LOG", rust_log) | ||
| .stderr_to_stdout() | ||
| .stdout_capture() | ||
| .unchecked() | ||
| .run() | ||
| .expect("Error running wasm client via node"); | ||
|
|
||
| status_ok_or_panic(output, run_command, "(running web)"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Yeah, I don't think this belongs here at all. The web target isn't some special magic that deserves special handling by the test framework any more than Unreal is. The test suite should just supply a run_command which contains an appropriate invocation of NodeJS.
| // The wasm/web SDK target runs under `wasm32-unknown-unknown`, where we do not | ||
| // have the native file APIs that back `with_debug_to_file`. Keeping the | ||
| // shared `extra_logging` field as `None` lets the rest of the connection and | ||
| // cache code stay unified without pretending that file logging works on web. | ||
| let extra_logging = None; |
There was a problem hiding this comment.
Why wouldn't we just log to the browser console, if files are unavailable?
| // The shared client implementation lives in `main.rs`, but only the wasm build | ||
| // needs to compile it as a library module. Native uses the binary entrypoint. | ||
| #[cfg(all(target_arch = "wasm32", feature = "web"))] | ||
| #[path = "main.rs"] | ||
| mod cli; |
There was a problem hiding this comment.
This is, uh, a weird way to avoid re-organizing this crate. I'd be much happier if we had:
main.rscontaining only the actual CLI parts.lib.rscontaining only the WASM entrypoint (andmoddecls).- A new file, probably called
test_handlers.rsor something, that contains the dispatch function and the meat.
Obviously this applies to all of the test clients, not just this one.
| #[cfg(all(target_arch = "wasm32", feature = "web"))] | ||
| static WEB_DB_NAME: OnceLock<String> = OnceLock::new(); | ||
|
|
||
| #[cfg(all(target_arch = "wasm32", feature = "web"))] | ||
| pub(crate) fn set_web_db_name(db_name: String) { | ||
| WEB_DB_NAME.set(db_name).expect("WASM DB name was already initialized"); | ||
| } |
There was a problem hiding this comment.
Just make the database name an additional argument to the tests, and have the CLI code read it out of the env var while the WASM entrypoint passes it normal.
| // `Timestamp::now()` is stubbed on `wasm32-unknown-unknown`, so client-side tests | ||
| // that need a timestamp value must use a deterministic literal instead of wall-clock time. |
There was a problem hiding this comment.
This should be a doc comment external to the function, not a regular comment interior to it.
| } | ||
|
|
||
| /// This tests that we can serialize and deserialize `Uuid` in various contexts. | ||
| fn exec_insert_caller_uuid_v7() {} | ||
| async fn exec_insert_caller_uuid_v7() {} |
There was a problem hiding this comment.
Are the UUID tests stubbed on master? Geez, they are. Don't love that.
| // TODO: Re-enable this once the wasm runner grows the browser-faithful mode | ||
| // described in `crates/testing/src/sdk.rs`. This test is about persisting web | ||
| // credentials across separate runs, and the current Node-based wasm harness does | ||
| // not exercise the browser cookie/storage APIs that web reauth depends on. |
| // See the TODO above and in `crates/testing/src/sdk.rs`: this is disabled for the | ||
| // current Node-based wasm harness for the same reason as `exec_reauth_part_1`. |
4 participants
Description of Changes
This PR adapts the Rust SDK test suite to work with the wasm version added in #4089.
This PR is based on that one, so to review just the test suite diff, do something like:
Most of the changes revolve around wasm's different async semantics - everything runs in one thread, so things that relied on background threads didn't work directly.
API and ABI breaking changes
None, I think/hope.
Expected complexity level and risk
2
Testing
webfeature